Event Notification DA Fully configurable #395

whoffler · 2025-03-04T10:23:42Z

Description

for issue - https://github.ibm.com/GoldenEye/issues/issues/12573

Event Notification DA Fully configurable flavor

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (X.x.x)

Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

solutions/fully-configurable/main.tf

ocofaigh · 2025-03-11T09:55:52Z

@whoffler Vipin has a PR open to update the current standard variation with some best practises. Since that variation is going away, can I suggest we just make the changes directly in this PR in the new variation you are adding?

ocofaigh

Left an initial first round of comments to address - there will be more. Also we need to claify if we really should be using cloud_object_storage and key_management_service in our input names, as opposed to just cos and kms. Discussion started on slack

README.md

ocofaigh · 2025-03-13T15:19:05Z

solutions/security-enforced/version.tf

@@ -0,0 +1,14 @@
+terraform {
+  required_version = ">= 1.3.0"


I suggest you update to >= 1.9.0 so we can use the terraform 1.9 cross variable validation feature.
We will need to use it in many input variables in variables.tf. For an idea of how to use it, see my branch (WIP): https://github.com/terraform-ibm-modules/terraform-ibm-scc/blob/DA/solutions/fully-configurable/variables.tf

solutions/security-enforced/version.tf

solutions/security-enforced/DA-cbr_rules.md

solutions/fully-configurable/variables.tf

solutions/fully-configurable/main.tf

solutions/fully-configurable/catalogValidationValues.json.template

solutions/fully-configurable/README.md

ocofaigh

@whoffler still a lot of things to address here. Its not consistent at all with the SCC DA. Please review your variable descriptions, variable validation and other logic to ensure consistency. Some other things that stand out:

security-enforced variation needs to be a striaght wrapper around fully-configurable. See https://github.com/terraform-ibm-modules/terraform-ibm-scc/tree/main/solutions/security-enforced as a reference
It seems we are supporting creation of cross regional buckets. We are not doing that anywhere else right now, so I think remove it so its less confusing for consumers

solutions/fully-configurable/variables.tf

solutions/fully-configurable/main.tf

ibm_catalog.json

ocofaigh

@whoffler Also take a look at terraform-ibm-modules/terraform-ibm-secrets-manager#309 - we will need something similar in this PR

whoffler · 2025-03-25T10:43:19Z

@ocofaigh

security-enforced variation needs to be a striaght wrapper around fully-configurable. See https://github.com/terraform-ibm-modules/terraform-ibm-scc/tree/main/solutions/security-enforced as a reference

was going to do that in a separate PR but will add here instead

ocofaigh · 2025-03-25T11:09:42Z

@whoffler A separate Pr is fine if you want - but just remove the security-enforced code from this PR then

rajatagarwal-ibm · 2025-04-11T16:21:01Z

/run pipeline

rajatagarwal-ibm · 2025-04-11T20:31:24Z

/run pipeline

rajatagarwal-ibm · 2025-04-15T13:54:01Z

/run pipeline

rajatagarwal-ibm · 2025-04-16T10:56:56Z

/run pipeline

rajatagarwal-ibm · 2025-04-16T12:19:29Z

/run pipeline

whoffler · 2025-05-06T12:44:03Z

/run pipeline

whoffler · 2025-05-06T12:51:47Z

/run pipeline

whoffler · 2025-05-06T14:19:58Z

/run pipeline

whoffler · 2025-05-06T15:06:41Z

/run pipeline

whoffler · 2025-05-07T13:21:03Z

/run pipeline

whoffler · 2025-05-07T13:49:32Z

/run pipeline

whoffler · 2025-05-07T15:04:34Z

/run pipeline

whoffler · 2025-05-07T15:17:26Z

/run pipeline

whoffler · 2025-05-07T16:07:08Z

/run pipeline

akocbek

in main variables.tf we have


variable "kms_encryption_enabled" {
  type        = bool
  description = "Set to `true` to control the encryption keys that are used to encrypt the data that you store in the Event Notifications instance. If set to `false`, the data is encrypted by using randomly generated keys. For more information, see [Managing encryption](https://cloud.ibm.com/docs/event-notifications?topic=event-notifications-en-managing-encryption)."
  default     = false
  validation {
    condition     = var.kms_encryption_enabled == false || var.plan == "standard"
    error_message = "kms encryption is only supported for standard plan"
  }
}

var.plan == "standard" is not included in description
variable description, validation is not in sync what we have in

terraform-ibm-event-notifications/solutions/fully-configurable/variables.tf

Line 117 in 09fd83d

variable "kms_encryption_enabled" {

akocbek · 2025-05-07T15:23:18Z

solutions/fully-configurable/README.md

+
+When `existing_en_instance_crn` is not passed, this solution configures the following infrastructure:
+
+- optionally a KMS key ring


what defines optionally (what inputs define if it is optional or not)?

if a user sets kms_encryption_enabled to true and provides an existing KMS instance then a KMS key ring will be created

akocbek · 2025-05-07T15:25:05Z

solutions/fully-configurable/README.md

+
+When `existing_en_instance_crn` is passed, this solution ignores ALL other inputs and sets the outputs based on the CRN.
+
+- required inputs MUST still be set, but will be ignored.


what is the purpose of setting required inputs if they are ignored?

think this is no longer true, will remove

akocbek · 2025-05-07T15:47:39Z

solutions/fully-configurable/main.tf

-  create_cross_account_en_kms_auth_policy = var.existing_en_instance_crn == null && !var.skip_en_kms_auth_policy && var.ibmcloud_kms_api_key != null
-  # Create cross account COS / KMS auth policy if not using existing EN instance, if not using existing bucket, if 'skip_cos_kms_auth_policy' is false, and if a value is passed for 'ibmcloud_kms_api_key'
-  create_cross_account_cos_kms_auth_policy = var.existing_en_instance_crn == null && var.existing_cos_bucket_name == null && !var.skip_cos_kms_auth_policy && var.ibmcloud_kms_api_key != null
+  kms_region                = var.kms_encryption_enabled ? var.existing_kms_instance_crn != null ? module.existing_kms_crn_parser[0].region : var.existing_kms_root_key_crn != null ? module.existing_kms_key_crn_parser[0].region : null : null


these are quite difficult to read.

could we improve it as the following (must be tested out)

kms_crn_parser = var.kms_encryption_enabled && ( var.existing_kms_instance_crn != null ? module.existing_kms_crn_parser[0] : var.existing_kms_root_key_crn != null ? module.existing_kms_key_crn_parser[0] : null ) kms_region = local.kms_crn_parser != null ? local.kms_crn_parser.region : null existing_kms_guid = local.kms_crn_parser != null ? local.kms_crn_parser.service_instance : null kms_service_name = local.kms_crn_parser != null ? local.kms_crn_parser.service_name : null kms_account_id = local.kms_crn_parser != null ? local.kms_crn_parser.account_id : null en_kms_key_id = local.create_kms_keys ? module.kms[0].keys[format("%s.%s", local.en_key_ring_name, local.en_key_name)].key_id : var.existing_kms_root_key_crn != null ? module.existing_kms_key_crn_parser[0].resource : null existing_kms_instance_crn = var.kms_encryption_enabled ? (var.existing_kms_instance_crn != null ? var.existing_kms_instance_crn : format("crn:v1:bluemix:%s:%s:%s:%s:%s::", module.existing_kms_key_crn_parser[0].ctype, module.existing_kms_key_crn_parser[0].service_name, module.existing_kms_key_crn_parser[0].region, module.existing_kms_key_crn_parser[0].scope, module.existing_kms_key_crn_parser[0].service_instance)) : null

akocbek · 2025-05-07T15:52:42Z

solutions/fully-configurable/main.tf

 }

 # If existing KMS root key CRN passed, parse details from it
-module "kms_root_key_crn_parser" {
+module "existing_kms_key_crn_parser" {


should we check if var.kms_encryption_enabled is true as well, since we did it for existing_kms_crn_parser

on the other hand, do we even need to check for this?

No we no longer need to check for this, removing

solutions/fully-configurable/main.tf

akocbek · 2025-05-07T16:07:55Z

solutions/fully-configurable/variables.tf

-  default     = null
-  description = "The CRN of an IBM Cloud Monitoring instance used to monitor the IBM Cloud Object Storage bucket that is used for storing failed events. If no value passed, metrics are sent to the instance associated to the container's location unless otherwise specified in the Metrics Router service configuration. Ignored if using existing Object Storage bucket."
-}
-
 variable "prefix" {
  type        = string


I think we decided prefix definition. Please take a look

https://github.ibm.com/GoldenEye/issues/issues/13592#issuecomment-112076767

prefix definition already in place?

akocbek · 2025-05-07T16:09:21Z

solutions/fully-configurable/variables.tf

+    condition = alltrue([
+      for tag in var.event_notifications_access_tags : can(regex("[\\w\\-_\\.]+:[\\w\\-_\\.]+", tag)) && length(tag) <= 128
+    ])
+    error_message = "Tags must match the regular expression \"[\\w\\-_\\.]+:[\\w\\-_\\.]+\", see https://cloud.ibm.com/docs/account?topic=account-tag&interface=ui#limits for more details"


should we add explanation what regex mean? not sure if every consumer would understand regex.

akocbek · 2025-05-07T16:13:21Z

solutions/fully-configurable/variables.tf

 }

-variable "kms_endpoint_url" {
+variable "existing_kms_root_key_crn" {


can existing_kms_root_key_crn be passed (is taken into account) in a case that kms_encryption_enabled is disabled? should we add this into input validation?

added input validation in the kms_encryption_enabled variable

akocbek · 2025-05-07T16:13:37Z

solutions/fully-configurable/variables.tf

  }
 }

-variable "existing_kms_root_key_crn" {
+variable "kms_endpoint_url" {


can kms_endpoint_url be passed (is taken into account) in a case that kms_encryption_enabled is disabled? should we add this into input validation?

added input validation in the kms_encryption_enabled variable

whoffler · 2025-05-07T18:01:51Z

/run pipeline

whoffler · 2025-05-07T18:50:48Z

/run pipeline

whoffler · 2025-05-07T18:57:19Z

/run pipeline

whoffler · 2025-05-07T19:55:08Z

/run pipeline

ocofaigh · 2025-05-07T20:35:51Z

@whoffler Can you share the error your getting before re-running the pipeline each time so we can track the failures?

whoffler · 2025-05-08T07:11:36Z

Sure, always getting the same error on different tests

summary: 'CreateIntegrationWithContext failed: Internal server error.'

whoffler · 2025-05-08T07:11:46Z

/run pipeline

addressed

terraform-ibm-modules-ops · 2025-05-08T10:36:48Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

GitHub release
v2.0.0

Your semantic-release bot 📦🚀

whoffler force-pushed the flexibleVariation branch 4 times, most recently from fd9cef1 to 5c55830 Compare March 5, 2025 11:57

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

ocofaigh mentioned this pull request Mar 11, 2025

refactor: da best input practices #393

Closed

6 tasks

whoffler changed the title ~~Event Notification DA Fully configurable - Work In Progress~~ Event Notification DA Fully configurable Mar 13, 2025

ocofaigh requested changes Mar 13, 2025

View reviewed changes

whoffler force-pushed the flexibleVariation branch 2 times, most recently from abeb33c to dcf9464 Compare March 20, 2025 09:08

whoffler requested a review from ocofaigh March 20, 2025 09:25

whoffler force-pushed the flexibleVariation branch from 29995b7 to 8d7f1f9 Compare March 20, 2025 11:04

ocofaigh requested changes Mar 24, 2025

View reviewed changes

ocofaigh reviewed Mar 24, 2025

View reviewed changes

ibm_catalog.json Outdated Show resolved Hide resolved

ocofaigh reviewed Mar 24, 2025

View reviewed changes

whoffler force-pushed the flexibleVariation branch 2 times, most recently from 71ea249 to b3f69cf Compare March 27, 2025 10:42

rajatagarwal-ibm marked this pull request as ready for review April 11, 2025 16:19

rajatagarwal-ibm requested review from Ak-sky and Aashiq-J as code owners April 11, 2025 16:19

whoffler force-pushed the flexibleVariation branch from 248f729 to fab02f3 Compare May 6, 2025 12:51

whoffler force-pushed the flexibleVariation branch from fab02f3 to 2448f91 Compare May 6, 2025 14:19

whoffler force-pushed the flexibleVariation branch from 2448f91 to f8e1e47 Compare May 7, 2025 13:12

SKIP UPGRADE TEST fully configurable solution

09fd83d

whoffler force-pushed the flexibleVariation branch from f8e1e47 to 09fd83d Compare May 7, 2025 15:17

akocbek previously requested changes May 7, 2025

View reviewed changes

improved validation, removed redundant checks

a51d3ee

ocofaigh approved these changes May 8, 2025

View reviewed changes

ocofaigh merged commit 27121e7 into main May 8, 2025
2 checks passed

ocofaigh deleted the flexibleVariation branch May 8, 2025 10:36

terraform-ibm-modules-ops added the released label May 8, 2025

ocofaigh mentioned this pull request May 19, 2025

[terraform-ibm-event-notifications] Add support for access tag #400

Closed


		When `existing_en_instance_crn` is not passed, this solution configures the following infrastructure:

		- optionally a KMS key ring


		When `existing_en_instance_crn` is passed, this solution ignores ALL other inputs and sets the outputs based on the CRN.

		- required inputs MUST still be set, but will be ignored.

Event Notification DA Fully configurable #395

Event Notification DA Fully configurable #395

Uh oh!

Conversation

whoffler commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocofaigh commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocofaigh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

whoffler commented Mar 25, 2025

Uh oh!

ocofaigh commented Mar 25, 2025

Uh oh!

rajatagarwal-ibm commented Apr 11, 2025

Uh oh!

rajatagarwal-ibm commented Apr 11, 2025

Uh oh!

rajatagarwal-ibm commented Apr 15, 2025

Uh oh!

rajatagarwal-ibm commented Apr 16, 2025

Uh oh!

rajatagarwal-ibm commented Apr 16, 2025

Uh oh!

whoffler commented May 6, 2025

Uh oh!

whoffler commented May 6, 2025

Uh oh!

whoffler commented May 6, 2025

Uh oh!

whoffler commented May 6, 2025

Uh oh!

whoffler commented May 7, 2025

Uh oh!

whoffler commented May 7, 2025

Uh oh!

whoffler commented May 7, 2025

Uh oh!

whoffler commented May 7, 2025

whoffler commented Mar 4, 2025 •

edited

Loading

ocofaigh commented Mar 11, 2025 •

edited

Loading

ocofaigh left a comment •

edited

Loading